Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(stats): track gh calls #859

Merged
merged 5 commits into from
Jul 28, 2023
Merged

feat(stats): track gh calls #859

merged 5 commits into from
Jul 28, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Jul 25, 2023 •
edited
Loading

Problem

Adds metric tracking for pa-corp.

Solution

  1. add statsD call for new endpoint
  2. get the trial site as a const in code - not very tied to this, we could do either env var or db table instead of this but my understanding is that the trial site is unlikely to change + we are implementing ld soon, which makes the extra effort not worth.

see the data here

Tests

  • go to pa-corp
  • make an edit
  • refresh
  • go back to datadog
  • number shuold have changed (might have to wait abit)

@seaerchin seaerchin requested a review from a team July 25, 2023 03:56
@seaerchin seaerchin marked this pull request as ready for review July 25, 2023 03:57
kishore03109
kishore03109 previously approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Non-nit, feel quite strongly abt this]: can we please not commit to agency websites in production for testing? Lets use a test repo for this ya

[nit] To facilitate this ^ I suggest moving GITHUB_EXPERIMENTAL_TRAIL_SITE into the constants.ts file and make it into an array and then we can map this over.

Since you have already tested this I am ok with the code as is, but this should not be the test case during our production

@seaerchin
Copy link
Contributor Author

seaerchin commented Jul 25, 2023 •
edited
Loading

@kishore03109 to be clear, we are releasing the github changes only to pa-corp, so i did this to be sure that it works as expected. re: array vs single const - no strong opinion, @harishv7 will ld come out anytime soon/do we have plans to shift to more than 1 site?

we could potentially extend this to every site and just remove the if condition tbh, but i'd have to check w tim on billing cos there was an incident in the past

@seaerchin seaerchin changed the title Feat/is 313 track gh calls feat(stats): track gh calls Jul 25, 2023
@kishore03109
Copy link
Contributor

kishore03109 commented Jul 25, 2023 •
edited
Loading

@seaerchin could we skip this test for release?
If we want to test functionality, how do you feel about env-varing it, test in some a-test-v4 and the change to pa-corp?

Mentioning this because have had agencies raise concern about us commenting on their repos before
either that or can just force push after to clean git history also can

@seaerchin
Copy link
Contributor Author

@seaerchin could we skip this test for release? If we want to test functionality, how do you feel about env-varing it, test in some a-test-v4 and the change to pa-corp?

Mentioning this because have had agencies raise concern about us commenting on their repos before either that or can just force push after to clean git history also can

ya, tested alr - feel free to skip for release. as mentioned in the PR description, i don't really mind either putting env var or table but didn't due to the reasons stated.

@kishore03109
Copy link
Contributor

@seaerchin sure, can j make a note in the pr desc that no need to test for release and merge alr bah

harishv7
harishv7 reviewed Jul 25, 2023
View reviewed changes
src/services/api/AxiosInstance.ts Show resolved Hide resolved
src/services/api/AxiosInstance.ts Outdated Show resolved Hide resolved
src/services/infra/StatsService.ts Show resolved Hide resolved
@seaerchin seaerchin requested a review from kishore03109 July 26, 2023 08:13
@seaerchin seaerchin dismissed kishore03109’s stale review July 26, 2023 08:13

stale - changed API on axios instance

@seaerchin seaerchin requested review from harishv7 and a team July 26, 2023 08:13
kishore03109
kishore03109 reviewed Jul 26, 2023
View reviewed changes
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanity check here, have we tested this with a test site with the new changes?

@harishv7
Copy link
Contributor

@kishore03109 to be clear, we are releasing the github changes only to pa-corp, so i did this to be sure that it works as expected. re: array vs single const - no strong opinion, @harishv7 will ld come out anytime soon/do we have plans to shift to more than 1 site?

we could potentially extend this to every site and just remove the if condition tbh, but i'd have to check w tim on billing cos there was an incident in the past

@seaerchin we plan to roll out to more sites upon evaluating the success of the MVP

@seaerchin
Copy link
Contributor Author

sanity check here, have we tested this with a test site with the new changes?

yes, see the dashboard here. also note that this was done from anotehr site (chin-test) to check functionality for multi-repo support

@kishore03109 kishore03109 self-requested a review July 27, 2023 06:37
@seaerchin seaerchin merged commit 7504012 into develop Jul 28, 2023
@seaerchin seaerchin deleted the feat/is-313-track-gh-calls branch July 28, 2023 04:45
@seaerchin seaerchin mentioned this pull request Aug 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kishore03109 kishore03109 kishore03109 approved these changes

@harishv7 harishv7 Awaiting requested review from harishv7

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seaerchin @kishore03109 @harishv7